Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Puppy Errors #485

Closed
wants to merge 1 commit into from
Closed

Puppy Errors #485

wants to merge 1 commit into from

Conversation

nicholascross
Copy link
Contributor

@nicholascross nicholascross commented Jun 24, 2019

Fixes #484

Review of colleague's PR #478

Changes proposed in this PR:

  • Create an executable go program
  • Copy the CRUD puppy from upstream master
  • Add a custom error type Error with fields Message and Code
  • Extend the Storer interface for all methods to also return error
  • Create errors for:
    • Value < 0
    • ID not found in Read, Update and Delete
  • Add locking for proper use of sync.Map

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #485 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #485   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          76     77    +1     
  Lines        1312   1385   +73     
=====================================
+ Hits         1312   1385   +73
Impacted Files Coverage Δ
07_errors/nicholascross/main.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a25446...163e60d. Read the comment docs.

Copy link
Collaborator

@n0npax n0npax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work mate!

1 blocking issue.
1 suggestion
1 architectural issue to be decided on what you want to do

07_errors/nicholascross/main.go Outdated Show resolved Hide resolved
07_errors/nicholascross/main.go Show resolved Hide resolved
07_errors/nicholascross/main.go Outdated Show resolved Hide resolved
07_errors/nicholascross/main_test.go Outdated Show resolved Hide resolved
 - Create an executable go program
 - Copy the CRUD puppy from upstream master
 - Add a custom error type `Error` with fields `Message` and `Code`
 - Extend the `Storer` interface for all methods to also return `error`
 - Create errors for:
     - Value < 0
     - ID not found in Read, Update and Delete
 - Add locking for proper use of sync.Map
Copy link
Collaborator

@n0npax n0npax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

Copy link
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some things can be simplified a bit and made a bit more idiomatic, but overall I like it. You may also have a couple of changes to incorporate from my lab 6 feedback.

}

func missingPuppy(id int64) error {
return &PupError{fmt.Sprintf("Puppy not found: %d", id), missingPup}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error strings should not start with a capital, as they are often wrapped and this leads to capitals in the middle of the message. See https://github.com/golang/go/wiki/Errors for more details.

This applies to the other two errors too.

}

func (e *PupError) Error() string {
return fmt.Sprintf("[%d] %s", e.Code, e.Message)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the code into the formatted string doesn't add any useful information because the code is meaningless to humans. The error string contains all the useful information. I'd suggest leaving it out.

}

func (ms MapStore) CreatePuppy(p Puppy) (bool, error) {
if p.Value < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about dogs, but I'm pretty sure my cat has negative value.

CreatePuppy(p Puppy) (bool, error)
RetrievePuppy(int64) (*Puppy, error)
UpdatePuppy(int64, Puppy) (bool, error)
DeletePuppy(int64) (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both a bool and error return value seems redundant. Whenever the bool is true, error is nil and vice versa. You can just return an error, and all is good if it is nil.

This applies to CreatePuppy, UpdatePuppy and DeletePuppy

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(p.ID); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use LoadOrStore (https://golang.org/pkg/sync/#Map.LoadOrStore) without needing the mutex (and also simpler code):

if _, ok := s.LoadOrStore(p.ID, p); ok {
        return false, puppyAlreadyExists(p.ID)
}
return true, nil

Even if this is run concurrently with one of the methods that takes the mutex, they do not require the puppy not to exist between the two sync.Map operations so CreatePuppy done this way does not interfere.

Although as I think about it, perhaps keeping the mutex in this method is better as it is clearer. However, LoadOrStore is still a shorter way to implement this.

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(id); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the happy path to the left edge (https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88) by inverting the sense of the conditional:

So,

if _, ok := s.store.Load(id); !ok {
        return false, missingPuppy(id)
}
p.ID = id
s.store.Store(p.ID, p)
return true, nil

s.lock.Lock()
defer s.lock.Unlock()

if _, ok := s.store.Load(id); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, align the happy path to the left:

if _, ok := s.store.Load(id); !ok {
        return false, missingPuppy(id)
}
s.store.Delete(id)
return true, nil

type MapStore map[int64]Puppy

const (
missingPup = iota
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Pup in these error codes and in the error type, but using Puppy everywhere is inconsistent. It would help if you used Puppy everywhere instead of Pup.


type PupError struct {
Message string
Code int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than one error type with a code to differentiate the errors, Go tends towards defining different error types for different errors named FooError for parameterised errors, or package-level variables that are error values created with errors.New() named ErrFoo for static errors.

See https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L230 for an example of the former and https://github.com/golang/go/blob/master/src/encoding/gob/decode.go#L19 for an example of the latter.

Or just create them on the fly as needed: https://github.com/golang/go/blob/master/src/database/sql/sql.go#L542

https://github.com/golang/go/wiki/Errors also has a bit more to say about this too.

I think the main thing is that your errors don't look like errors (from a Go perspective). return nil, missingPuppy(id) does not say err(or) anywhere which is what I would usually expect to see in Go code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it looks like I didn't read the lab description where it says "Add a custom error type Error with fields Message and Code" - so ignore my comment about using Code (or not, as you please). I think my last paragraph in this comment still stands though - the errors don't look like errors, so if you can put "[Ee]rr" in the names somewhere I think it would read better.

@juliaogris
Copy link
Contributor

The go-course is now closed. Thank you very much for participating.

@juliaogris juliaogris closed this Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lab 7 - Errors
5 participants